feat: add authz dependency injections#1539
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## tiled-token-validation #1539 +/- ##
==========================================================
- Coverage 95.79% 95.75% -0.04%
==========================================================
Files 44 44
Lines 3280 3325 +45
==========================================================
+ Hits 3142 3184 +42
- Misses 138 141 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
shree-iyengar-dls
changed the title
tpoliaw
force-pushed
the
add_depedency_injection
branch
from
27a9865 to
ae58c78
Compare
tpoliaw
force-pushed
the
tiled-token-validation
branch
from
b575c2b to
b6049c8
Compare
tpoliaw
force-pushed
the
add_depedency_injection
branch
from
ae58c78 to
8112e4b
Compare
ZohebShaikh
left a comment
ZohebShaikh
left a comment
There was a problem hiding this comment.
Looks Good, few comments
| from blueapi.service.model import TaskRequest | ||
|
|
||
| LOGGER = logging.getLogger(__name__) | ||
| INSTRUMENT_SESSION_RE = re.compile(r"^[a-z]{2}(?P<proposal>\d+)-(?P<visit>\d+)$") |
There was a problem hiding this comment.
nit: Can we use the same value here ? Maybe move it to a constants file and import it from there
I had a look at the ISypB database they have all lower-case first 2 characters. So the above link regex should drop A-Z
We never make any check on the code(cm,sm..) of the visit, so why bother checking if it is 2 character and a-z
because it will pass even if you write cm12345-1 or ab12345-1
There was a problem hiding this comment.
I've moved it to the root of utils for now until there are enough constants to be worth a module.
I think it makes sense to keep the prefix check to ensure that 123-456 isn't accepted as a valid visit. We might be able to confirm the correct valid in future.
tpoliaw
force-pushed
the
tiled-token-validation
branch
from
b6049c8 to
63009c0
Compare
tpoliaw
force-pushed
the
add_depedency_injection
branch
from
8112e4b to
a4c6778
Compare
tpoliaw
force-pushed
the
tiled-token-validation
branch
from
63009c0 to
9865265
Compare
tpoliaw
force-pushed
the
add_depedency_injection
branch
from
a4c6778 to
4fa20de
Compare
| ): | ||
| task = runner.run(interface.get_task_by_id, task_id) | ||
|
|
||
| if opa and not opa.admin() and (task and fedid != task.task.metadata.get("user")): |
There was a problem hiding this comment.
This only checks task access if OPA is configured. Do we want to restrict access based on user name when authn is enabled but authz is not?
| from blueapi.service.model import TaskRequest | ||
|
|
||
| LOGGER = logging.getLogger(__name__) | ||
| INSTRUMENT_SESSION_RE = re.compile(r"^[a-z]{2}(?P<proposal>\d+)-(?P<visit>\d+)$") |
There was a problem hiding this comment.
I've moved it to the root of utils for now until there are enough constants to be worth a module.
I think it makes sense to keep the prefix check to ensure that 123-456 isn't accepted as a valid visit. We might be able to confirm the correct valid in future.
3 participants
https://jira.diamond.ac.uk/browse/ACQP-551